Skip to content

Allow disabling auto-reload#155

Merged
harshavardhana merged 1 commit intomainfrom
disable-auto-reload
May 1, 2025
Merged

Allow disabling auto-reload#155
harshavardhana merged 1 commit intomainfrom
disable-auto-reload

Conversation

@ramondeklein
Copy link
Copy Markdown
Contributor

@ramondeklein ramondeklein commented Mar 20, 2025

The current certs.Manager only allows reloading of existing certificates. These certificates are reloaded when the certificate file (on disk) is changed or when the manager receives SIGHUP.

For AIStor operator, we should also support adding new certificates. The sidecar will copy the certificates to the appropriate location and instruct MinIO to reload ALL certificates by sending SIGHUP. By disabling the auto-reload functionality in certs.Manager, reloading the certificates twice (and possible dangling Go-routines) will be prevented.

If certs.WithDisableAutoReload() is not added, then functionality is still the same.

This is required for https://github.com/miniohq/eos/pull/554.

@ramondeklein ramondeklein added the enhancement New feature or request label Mar 20, 2025
@ramondeklein ramondeklein self-assigned this Mar 20, 2025
@ramondeklein ramondeklein marked this pull request as ready for review March 26, 2025 18:32
Comment thread certs/manager.go
Comment on lines -71 to +72
func NewManager(ctx context.Context, certFile, keyFile string, loadX509KeyPair LoadX509KeyPairFunc) (manager *Manager, err error) {
func NewManager(ctx context.Context, certFile, keyFile string, loadX509KeyPair LoadX509KeyPairFunc, opts ...func(*Manager)) (manager *Manager, err error) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I choose this option (instead of passing a disableAutoReload parameter) to ensure the API doesn't break. If this isn't an issue, we may want to pass a parameter instead.

@ramondeklein
Copy link
Copy Markdown
Contributor Author

@aead @harshavardhana I would like to go ahead with my SIGHUP reload PR. It's done, but this PR needs to be merged first. I think it's fully backwards compatible, but I would like to have your blessing and get this merged. Then we can review/approve https://github.com/miniohq/eos/pull/554.

@ramondeklein
Copy link
Copy Markdown
Contributor Author

@aead @harshavardhana Can you review? We need this fix to allow the operator sidecars to reload all certificates when the ObjectStore configuration changes.

@ramondeklein
Copy link
Copy Markdown
Contributor Author

@harshavardhana Do you know anyone else who can review this PR? I would like to improve the auto-reloading of certificates in Kubernetes without restarting. I had to restart a customer's statefulset today again, because they added an external certificate and it wasn't picked up.

@harshavardhana harshavardhana merged commit 3a9e546 into main May 1, 2025
3 checks passed
@harshavardhana harshavardhana deleted the disable-auto-reload branch May 1, 2025 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants